Skip to content

Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount#1578

Merged
cedric-cordenier merged 8 commits intomainfrom
PRIV-198-domain-sep
Oct 14, 2025
Merged

Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount#1578
cedric-cordenier merged 8 commits intomainfrom
PRIV-198-domain-sep

Conversation

@vreff
Copy link
Copy Markdown
Contributor

@vreff vreff commented Oct 2, 2025 •
edited
Loading

This ensures that users of the StandardCapabilityAccount account for the SignerDecrypter component domain separate their payloads using the recommended MakePeerIDSignatureDomainSeparatedPayload helper function from libocr.

Also, bumps libocr.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 2, 2025 •
edited
Loading

✅ API Diff Results - No breaking changes

📄 View full apidiff report | 📚 Learn about apidiff

@vreff vreff changed the title Require domain separation for SignerDecrypter signature payloads Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount Oct 8, 2025
@vreff vreff marked this pull request as ready for review October 8, 2025 22:27
@vreff vreff requested a review from a team as a code owner October 8, 2025 22:27
jmank88
jmank88 reviewed Oct 9, 2025
View reviewed changes
Comment thread pkg/types/core/keystore.go Outdated
@vreff @jmank88
Update pkg/types/core/keystore.go
5b09c13 
Co-authored-by: Jordan Krage <jmank88@gmail.com>
github-actions[bot]
github-actions Bot reviewed Oct 9, 2025
View reviewed changes
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 3d2884e Previous: 60798d1 Ratio
BenchmarkKeystore_Sign/nop/in-process 731.8 ns/op 352.2 ns/op 2.08

This comment was automatically generated by workflow using github-action-benchmark.

@vreff
Copy link
Copy Markdown
Contributor Author

vreff commented Oct 9, 2025

@jmank88 interesting to see: #1578 (review).

I guess bytes.hasPrefix is a bit less performant than the manual check. I don't foresee this being a problem for our use case, LMK if there's any reason we should care about the change in signing speed.

@jmank88
Copy link
Copy Markdown
Contributor

jmank88 commented Oct 9, 2025

@jmank88 interesting to see: #1578 (review).

I guess bytes.hasPrefix is a bit less performant than the manual check. I don't foresee this being a problem for our use case, LMK if there's any reason we should care about the change in signing speed.

Oh, wasn't that there with the last version too? The internals look equivalent.

@vreff
Copy link
Copy Markdown
Contributor Author

vreff commented Oct 9, 2025

Oh, wasn't that there with the last version too? The internals look equivalent.

The benchmarks test passed on the commit before the hasPrefix change 🤷

@cedric-cordenier cedric-cordenier enabled auto-merge (squash) October 14, 2025 13:20
@cedric-cordenier cedric-cordenier merged commit 5662ca1 into main Oct 14, 2025
23 of 25 checks passed
@cedric-cordenier cedric-cordenier deleted the PRIV-198-domain-sep branch October 14, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@nadahalli nadahalli nadahalli approved these changes

@jmank88 jmank88 jmank88 approved these changes

@cedric-cordenier cedric-cordenier cedric-cordenier approved these changes

@patrickhuie19 patrickhuie19 Awaiting requested review from patrickhuie19

@nolag nolag Awaiting requested review from nolag

@DylanTinianov DylanTinianov Awaiting requested review from DylanTinianov

@pavel-raykov pavel-raykov Awaiting requested review from pavel-raykov

@huangzhen1997 huangzhen1997 Awaiting requested review from huangzhen1997

@ibrajer ibrajer Awaiting requested review from ibrajer

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vreff @jmank88 @nadahalli @cedric-cordenier